add simple check for variables with different capitalization #84
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #84 +/- ##
==========================================
+ Coverage 41.41% 41.65% +0.23%
==========================================
Files 51 51
Lines 1671 1707 +36
==========================================
+ Hits 692 711 +19
- Misses 979 996 +17 ☔ View full report in Codecov by Sentry. |
fbenke-pik
force-pushed
the
variableCheck
branch
from
58f0e85
to
64856f7
Compare
|
Managed to bring down the runtime to approximately 75 seconds by avoiding to grep the entire codebase twice. Instead, I do it only once with ignore.case =T and then apply a second search using ignore.case = F on the intermediate search instead of doing it on the whole codebase again, as the latter is a subset of the former. For future reference, the new check could also be implemented using stringr::str_extract_all instead of grep, but it seems to take much longer and miss some valid results, so going with grep for now |
|
@tscheypidi I can add a list of lines per found name, but it seems a bit tricky to print a meaningful warning, as the number of affected lines can be quite long and the warnings will be truncated: How about introducing a helper that accepts a variable name and returns the respective lines and is run outside of the check instead? Or better use an ignore list after all? |
|
I think this list of lines can be useful even if partly truncated as it gives you an indication what kind of instances were found. |
|
Ok, I have added the detailed message. Are we good to go then? |
|
I added support for an exclusion list. See also magpiemodel/magpie#641 |
R/codeCheck.R
Outdated
| ap <- checkAppearance(gams) | ||
| capitalExclusionList <- NULL | ||
| if (file.exists(file.path(path, ".codeCheck.yaml"))) { | ||
| capitalExclusionList <- read_yaml(file.path(path, ".codeCheck.yaml"))[["capitalExclusionList"]] |
There was a problem hiding this comment.
2 Suggestions:
- can we use the name
.codeCheckinstead of.codeCheck.yaml? This seems to me more consistent to all the other config files like.lintr,.Rprofile,.buildLibraryand so on. - Can you add some information about
.codeCheckin the documentation of codeCheck so that users are aware of it and know which settings are available?
| capitalExclusionList <- read_yaml(file.path(path, ".codeCheck.yaml"))[["capitalExclusionList"]] | |
| capitalExclusionList <- read_yaml(file.path(path, ".codeCheck"))[["capitalExclusionList"]] |
Implements a possible solution to address #82